Feature: switch visibility with update_repo_settings #2537 #2541

WizKnight · 2024-09-14T09:00:15Z

This PR enhances the update_repo_settings method to provide more control over repository settings, and deprecates the update_repo_visibility method:

Key changes:

Added a private parameter to set repositories as public or private.
Made the gated and private parameters optional, allowing updates to either setting independently.
Added checks to ensure at least one of gated or private is provided.
Payload Optimization: gated and private are only included in the request payload if they have non-None values.
Updated the docstring for update_repo_settings and replaced all references to update_repo_visibility in the documentation.
Added the @_deprecate_method decorator to update_repo_visibility, marking it for removal in v0.29.x.

WizKnight · 2024-09-14T09:49:06Z

Hey @Wauplin, the changes in this PR are implemented as per the TO-DOs in issue #2537.

hanouticelina

Hi @WizKnight! thank you for working on this 🤗 I left some comments and I think they are still other references of update_repo_visibility in other files and docs to update

src/huggingface_hub/hf_api.py

src/huggingface_hub/utils/__init__.py

src/huggingface_hub/hf_api.py

WizKnight · 2024-09-19T15:49:46Z

Hey @hanouticelina :), changes were added in the recent commit.

HuggingFaceDocBuilderDev · 2024-09-20T09:45:08Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

hanouticelina

Hi @WizKnight! thanks a lot for this iteration! 😄 I left a small comment and there is also this one that needs to be addressed.
I just realized that update_repo_visibility() is used in other tests in tests/test_file_download.py and tests/test_snapshot_download.py. Could you replace update_repo_visibility() with update_repo_settings() in those files as well?
The failing test seems unrelated, I'll take a look

src/huggingface_hub/hf_api.py

Wauplin · 2024-09-20T14:29:40Z

I just realized that update_repo_visibility() is used in other tests in tests/test_file_download.py and tests/test_snapshot_download.py. Could you replace update_repo_visibility() with update_repo_settings() in those files as well?

For deprecated methods, we have a expect_deprecation decorator that can be used in tests (see here). The goal is to continue to have the method tested since it's still officially supported and we are also testing that the method is correctly deprecated (i.e. correctly triggers warning).

So in the case of the deprecate use of it here, you can do

    @expect_deprecation("update_repo_visibility")
    def test_download_private_model(self):
        self.api.update_repo_visibility(repo_id=self.repo_id, private=True)

(+ adapt the import)

(sorry it's the kind of undocumentated things...)

Wauplin

Thanks @WizKnight! Here are a few changes I'd make in addition to @hanouticelina's comment above :)

tests/test_hf_api.py

src/huggingface_hub/hf_api.py

Wauplin · 2024-09-20T14:40:54Z

src/huggingface_hub/hf_api.py

        headers = self._build_hf_headers(token=token)

+        # Preparing the JSON payload for the PUT request
+        payload = {"gated": gated, "private": private}


I would prepare the payload in 2 steps:

payload: Dict = {} if gated is not None: if gated not in ["auto", "manual", False]: raise ValueError(f"Invalid gated status, must be one of 'auto', 'manual', or False. Got '{gated}'.") payload["gated"] = gated if private is not None: payload["private"] = private

The advantage of doing so is that adding a new parameter will just be a matter of adding a new if statement. Also, it makes it easier to log/print the payload value in case of debugging an issue. Doing a {key: value for key, value in payload.items() if value is not None} is not wrong per-se but makes it slightly less readable and less debuggable

I was planning on a similar approach, but I opted for the dictionary comprehension {key: value for key, value in payload.items() if value is not None} to make the code more efficient.

Sometimes readibility / maintenance is more important than efficiency :)

Wauplin · 2024-09-20T14:44:22Z

docs/source/en/guides/repository.md


 >>> api = HfApi()
->>> api.update_repo_settings(repo_id=repo_id, gated="auto")  # Set automatic gating for a model
+>>> api.update_repo_settings(repo_id=repo_id, gated="auto", private=True)  # Set automatic gating and visibility for a model


I would structure the documentation as such:

## Change repository settings (...) current content ### Update visibility (...) current content + update example ### Setup gated access (...) current content

Setting a repo as gating and setting repo visibility is 2 different topics, even if related to accessibility. Usually either a repo is public, either private, either public and gated. Setting a repo as private and gated is possible but doesn't make sense.

hanouticelina

Hi @WizKnight ! thanks again for this 😄 I left two minor comments.

The tests failure comes from the test test_repo_id_no_warning() which is flagged with the expect_deprecation decorator and contains also a context manager warnings.catch_warnings and they both try to capture the same warning.
The FutureWarning is "consumed" by pytest.warns in the expect_deprecation decorator, so it never reaches the warnings.catch_warnings context manager inside the test. This is why the record list does not contain the FutureWarning and the test fails.

@Wauplin do we still need this test ? it tests that there is no warnings when passing repo_id as positional argument into some HfApi functions (create_repo, delete_repo, and update_repo_visibility).

hanouticelina · 2024-09-23T08:03:13Z

src/huggingface_hub/hf_api.py

            repo_id (`str`):
                A namespace (user or an organization) and a repo name separated by a /.
            gated (`Literal["auto", "manual", False]`, *optional*):
                The gated release status for the repository.


Suggested change

The gated release status for the repository.

The gated status for the repository. If set to `None` (default), the `gated` setting of the repository won't be updated

hanouticelina · 2024-09-23T08:03:35Z

src/huggingface_hub/hf_api.py

                * "auto": The repository is gated, and access requests are automatically approved or denied based on predefined criteria.
                * "manual": The repository is gated, and access requests require manual approval.
-                * False (default): The repository is not gated, and anyone can access it.
+                * False : The gated status for the repository. If set to `None` (default), the `gated` setting of the repository won't be updated.


Suggested change

* False : The gated status for the repository. If set to `None` (default), the `gated` setting of the repository won't be updated.

* False : The repository is not gated, and anyone can access it.

Wauplin · 2024-09-23T12:28:22Z

@Wauplin do we still need this test ? it tests that there is no warnings when passing repo_id as positional argument into some HfApi functions (create_repo, delete_repo, and update_repo_visibility).

Clearly not needed anymore indeed! Let's just delete it instead of trying to fix the warning capture thingy (we could extend expect_deprecation to handle multiple inputs but really not worth it)

WizKnight · 2024-09-23T14:26:06Z

HfApi functions (create_repo, delete_repo, and update_repo_visibility).

Hey @Wauplin and @hanouticelina, so do I remove these HfApi functions (create_repo, delete_repo, and update_repo_visibility) completely or extend those functions with expect_deprecation ??

Wauplin · 2024-09-23T16:15:18Z

No no, the only thing to do is to remove the test test_repo_id_no_warning. It's an outdated and legacy one. The methods create_repo and delete_repo are far from been deprecated!

WizKnight · 2024-09-26T11:10:15Z

Hey @Wauplin I removed the test test_repo_id_no_warning completely and iterated on @hanouticelina minor comments in the recent commit.

Wauplin · 2024-09-26T13:07:17Z

(I've switched the PR as "ready for review". In general @WizKnight you can switch a PR to "ready" as soon as you request someone to review it)

hanouticelina

Hey @WizKnight 👋 thanks a lot for this PR! looks good to me 🙂 failing tests are unrelated, let's wait for @Wauplin's final review and we will be good to merge

Wauplin

Looking good! Thanks for the back and forth @WizKnight @hanouticelina 🤗

Failing test is indeed unrelated. I left minor comments otherwise we are good to merge :)

docs/source/en/guides/repository.md

Wauplin · 2024-09-26T14:34:23Z

CI's green, let's merge 🎉 Thanks again @WizKnight 🤗

WizKnight · 2024-09-26T15:16:43Z

Hey @Wauplin & @hanouticelina , I really appreciate the opportunity!! Thankyou once again🤗

Enhance update_repo_settings to manage repo visibility

de6794c

WizKnight added 3 commits September 16, 2024 13:50

Merge branch 'main' into feature/switch-visibility-with-repo-settings

eae6028

Merge branch 'main' into feature/switch-visibility-with-repo-settings

5a376f3

Merge branch 'main' into feature/switch-visibility-with-repo-settings

5e3aef1

hanouticelina requested changes Sep 19, 2024

View reviewed changes

Enhance update_repo_settings to manage repo visibility

e169e96

WizKnight requested a review from hanouticelina September 19, 2024 16:53

hanouticelina requested changes Sep 20, 2024

View reviewed changes

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved

Wauplin reviewed Sep 20, 2024

View reviewed changes

WizKnight added 3 commits September 20, 2024 23:12

Merge branch 'main' into feature/switch-visibility-with-repo-settings

edf03d2

Enhance update_repo_settings to manage repo visibility

06e31db

Enhance update_repo_settings to manage repo visibility

b06e6b4

WizKnight requested review from Wauplin and hanouticelina September 20, 2024 20:15

hanouticelina requested changes Sep 23, 2024

View reviewed changes

WizKnight added 2 commits September 26, 2024 16:38

Enhance update_repo_settings to manage repo visibility

e68280c

Merge branch 'main' into feature/switch-visibility-with-repo-settings

93d87a6

WizKnight requested a review from hanouticelina September 26, 2024 11:10

Wauplin marked this pull request as ready for review September 26, 2024 13:06

hanouticelina approved these changes Sep 26, 2024

View reviewed changes

Wauplin approved these changes Sep 26, 2024

View reviewed changes

docs/source/en/guides/repository.md Outdated Show resolved Hide resolved

docs/source/en/guides/repository.md Outdated Show resolved Hide resolved

docs/source/en/guides/repository.md Outdated Show resolved Hide resolved

Apply suggestions from code review

8b13d08

Wauplin merged commit f984cdc into huggingface:main Sep 26, 2024
16 checks passed

Wauplin mentioned this pull request Sep 26, 2024

Feature: switch visibility with update_repo_settings #2537

Closed

	The gated release status for the repository.
	The gated status for the repository. If set to `None` (default), the `gated` setting of the repository won't be updated

	* False : The gated status for the repository. If set to `None` (default), the `gated` setting of the repository won't be updated.
	* False : The repository is not gated, and anyone can access it.

Feature: switch visibility with update_repo_settings #2537 #2541

Feature: switch visibility with update_repo_settings #2537 #2541

Uh oh!

Conversation

WizKnight commented Sep 14, 2024

Uh oh!

WizKnight commented Sep 14, 2024

Uh oh!

hanouticelina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WizKnight commented Sep 19, 2024

Uh oh!

HuggingFaceDocBuilderDev commented Sep 20, 2024

Uh oh!

hanouticelina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wauplin commented Sep 20, 2024

Uh oh!

Wauplin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wauplin Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

WizKnight Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

Wauplin Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wauplin Sep 20, 2024

Choose a reason for hiding this comment

Uh oh!

hanouticelina left a comment

Choose a reason for hiding this comment

Uh oh!

hanouticelina Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

hanouticelina Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Wauplin commented Sep 23, 2024

Uh oh!

WizKnight commented Sep 23, 2024

Uh oh!

Wauplin commented Sep 23, 2024

Uh oh!

WizKnight commented Sep 26, 2024

Uh oh!

Wauplin commented Sep 26, 2024

Uh oh!

hanouticelina left a comment

Choose a reason for hiding this comment

Uh oh!

Wauplin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Wauplin commented Sep 26, 2024

Uh oh!

Uh oh!

WizKnight commented Sep 26, 2024

Uh oh!

hanouticelina left a comment •

edited

Loading

Wauplin Sep 23, 2024 •

edited

Loading